perf: the interactive path - #54
Conversation
937047c to
5596a72
Compare
IAmJSD
left a comment
There was a problem hiding this comment.
This review was written by Claude (Fable 5), acting on Astrid's behalf.
The core algorithm work here is strong: the running-sum box blur, parallel lens blur/edges/warp/convolve/inpaint, disc-morphology decomposition, incremental seam-carve energy, scanline lasso fill, Felzenszwalb–Huttenlocher EDT, LRU tile caps and one-undo quick-select all check out as output-equivalent, and the equivalence tests inline the exact replaced implementations as references with real boundary cases. What blocks the merge is the app-shell glue around them:
-
Selection + generator filters regress (
workspace.rs,filter_region_with_context~4711): with a selection active, the region is now intersected withcontent_bounds().inflated(pad)(pad = 0 on the dialog path). Render ▸ Clouds on a new empty layer creates content —content_bounds()is EMPTY, the intersection is empty, andbegin_filter_previewrefuses with "Nothing to filter" where main fills the selection. A selection extending past existing content similarly gets its clouds cropped. -
The headline "filters read through the selection edge" fix is dead plumbing (
apply_filter~4925 /begin_filter_preview~4860):filter_region_with_contextwith context > 0 is unreachable — parameterless filters pass 0, and the dialog's Apply reusespreview.region, whichbegin_filter_previewcomputed ungrown viafilter_region. So the wholeFilterPlugin::context()surface (plugin-api, the blur impls, thesimple_filter!heuristic) never grows a buffer on any interactive path, and the selection-edge band the PR body claims to fix is still there. Thecontext_testsonly assert thatcontext()returns numbers. The growth needs to happen inbegin_filter_preview. -
Prefetch permanently disabled on large documents (
prefetch_tick~5464):if self.cache.is_full() { self.prefetch_queue.clear(); return false; }— steady state after eviction isbytes >= budget, so once a 16k×16k document fills the 256 MiB budget, every rebuilt nearest-first queue for a new viewport is discarded each tick, including the mid-gesture warming main's own comments call out as what makes the settle frame land instantly. The comment's premise ("what is already in is what the viewport actually needs") is wrong the moment the viewport moves. Prefetch the near ring and let LRU evict distant tiles.
Smaller items:
fx_blur.wgsl:4-7still claims the shader mirrorsbox_pass"tap for tap… no running total whose float error would drift" — now false; update the comment and ideally add a long-dimension parity case (drift grows with row length; the suite only tests 64×48).horizontal_morph's "monotonic deque, O(1) per pixel" is aVecwithremove(0)— O(window) per removal; make it aVecDeque.MAX_FILTER_CONTEXTwas inserted betweenAUTOSAVE_SECSand its doc comment (workspace.rs ~87), andbox_passhas a stale stacked doc comment — same splice pattern as sibling PRs.simple_filter!treating any"amount"param as spatial reach conflates intensity with displacement (Add Noise grows by up to 256 px once item 2 is fixed).cost_does_not_grow_with_the_radiusasserts wall-clock ratios — a CI flake hazard that mostly tests the new code against itself.- The cited perf numbers (205 ms → …, 5.62 s, 4.43 s) have no harness behind them despite the repo's
fxbench/afbenchprecedent — plausible given the complexity changes, but worth pinning.
Fix the three majors and this is a clear merge — the algorithmic core is the best-engineered part of this PR series.
dce95ab to
17ac2b2
Compare
|
fixed |
IAmJSD
left a comment
There was a problem hiding this comment.
This follow-up review was written by Claude (Fable 5), acting on Astrid's behalf.
Most of the round landed well: the generator/selection regression is properly fixed (selection.bounds().inflated(pad) with no content intersection — Clouds into a selection on an empty layer works), the context plumbing is now live end-to-end on the dialog path with consistent preview/commit blit math, the wgsl comment is honest with a 4096-row parity case, horizontal_morph is a real VecDeque, the wall-clock test became a semantic one, and the benches exist. Good work.
One new major keeps this from merging — the prefetch fix trades "permanently dead" for "actively evicts the viewport" on the same large documents:
prefetch_tick no longer stops when full (good), but three facts combine badly (workspace.rs:65, crates/compositor/src/lib.rs:722,840):
PREFETCH_TILE_BUDGET = 2048tiles ≈ 0.5–1 GiB, twice the cache's ~1024-tile capacity (256 MiB / 256 KiB per tile) — the queue's own comment says so.TileCache::prewarmfilters to missing tiles and never refreshes resident entries'touchedstamps; paints are served fromdisplay_tilesfirst, which skipscache.get— so nothing re-touches visible tiles while a drain runs.- Each insert past capacity evicts the minimum-stamp entry, and
display_tile's retain drops the display copies of whatever the composited cache evicts.
So on a 16k×16k document a full drain first evicts the viewport, then the near ring, then its own earliest (nearest) inserts; after ~8 s the resident set is the farthest ~1024 queued tiles, the next paint recomposites the whole viewport — exactly the stall prefetch exists to prevent — then rebuilds the queue and loops forever. The new comment's "LRU evicts the distant tiles instead" is inverted. Any one of these fixes it: cap the queue at (cache tile capacity − visible count); have the per-paint visible prewarm touch resident entries; or stop the drain when the next candidate is farther than the nearest would-be evictee.
Two smaller residuals, take or leave:
- The Filter Gallery path (
show_filter_gallery→ plainbegin_filter_preview(),workspace.rs:2493) still previews and commits with an ungrown region, so the selection-edge band this PR fixes for the dialog persists for the same filters applied via the gallery. - The dialog reach heuristic (
workspace.rs:5276) includes"amount", whichsimple_filter!::context()deliberately dropped one commit earlier — Add Noise previews grow by up to 256 px for a filter that reads nothing. Perf-only, but the two lists should agree.
Also still open from last round: the unbounded failed.join(", ") export status string, and no end-to-end test asserting the region growth actually reaches a filter (context_tests still only check context() numbers). Fix the prefetch inversion and this merges.
groups five branches plus the filter half of a sixth: work that ran on one core, on the ui thread, or with the wrong complexity.
gestures that froze the window
par_chunks_mutover rows, the same iteration spread over the cores.one drag with the quick selection tool, and what the history panel holds afterwards:
(the left panel is scrolled — there are more entries below it.)
wrong complexity
signed_distancescanned a(2r+1)²window per pixel, and photoshop's stroke and glow sizes go to 250. replaced with an exact euclidean distance transform — two 1-d passes, independent of radius.both are pinned by benches that time the old formulation against the new one, inlined the same way the equivalence tests carry it —
cargo run --release -p schist-layer-fx --example strokebenchand-p schist-fx --example cpubench. on this machine:memory with no ceiling
both tile caches were unbounded while the prefetcher deliberately warms the whole document — about 512 MB resident for an 8000² document and 2 GB for 16000². now a 256 MiB budget with lru eviction,
display_tilespruned to match, and the prefetcher stops at the budget instead of warming tiles that only evict each other.and two correctness fixes that live here
filters clamped at the selection's bounding box instead of reading the surrounding image, leaving a visible band along the selection edge on any large-radius blur;
FilterPluginnow advertises its reach and the shell hands over a grown buffer, with the write side still masked by the selection. region export swallowed both of its failure paths and reported however many had worked.how the equivalence is checked, not asserted
every algorithm swap here is pinned against the formulation it replaced, sample for sample:
the_scanline_fill_matches_the_brute_force_one(self-crossing star, triangle, concave L),the_distance_transform_matches_the_window_search(five limits),the_inpaint_matches_a_sequential_jacobi_solve,the_decomposed_disc_matches_a_full_disc_scan(five radii, both directions),incremental_energy_carves_the_same_seams(five targets, both directions).cargo fmt --all --check,cargo clippy --workspace --all-targets -D warnings,cargo test --workspace— 635 passed, 0 failed.how this relates to my other open prs
these seven are independent of each other — each branches off
mainand each is green on its own. they do share files with my five open prs (#36, #38, #42, #44, #46), mostlyworkspace.rs, so whichever lands first will leave the others needing a rebase. happy to rebase in whatever order suits you, or to split any of these further if one is too big to review in a sitting.